Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

unverified HTTPS: don't set CURLOPT_SSL_VERIFYHOST=0 #114

Merged
merged 2 commits into from
Apr 21, 2021

Conversation

StefanKarpinski
Copy link
Member

In https://curl.se/libcurl/c/CURLOPT_SSL_VERIFYHOST.html under "Limitations", it is documented that when CURLOPT_SSL_VERIFYHOST is set to zero this also turns off SNI (Server Name Indication):

Secure Transport: If verify value is 0, then SNI is also disabled. SNI is a TLS extension that sends the hostname to the server. The server may use that information to do such things as sending back a specific certificate for the hostname, or forwarding the request to a specific origin server. Some hostnames may be inaccessible if SNI is not sent.

Since SNI is required to make requests to some HTTPS servers, disabling SNI can break things. This change leaves host verification on and only turns peer verification off (i.e. CA chain checking). I have yet to find an example where turning host verification off is necessary.

and bugfix: setting `ENV[var] = nothing` does not delete the var.
In https://curl.se/libcurl/c/CURLOPT_SSL_VERIFYHOST.html under
"Limitations", it is documented that when `CURLOPT_SSL_VERIFYHOST` is
set to zero this also turns off SNI (Server Name Indication):

> Secure Transport: If verify value is 0, then SNI is also disabled. SNI
> is a TLS extension that sends the hostname to the server. The server
> may use that information to do such things as sending back a specific
> certificate for the hostname, or forwarding the request to a specific
> origin server. Some hostnames may be inaccessible if SNI is not sent.

Since SNI is required to make requests to some HTTPS servers, disabling
SNI can break things. This change leaves host verification on and only
turns peer verification off (i.e. CA chain checking). I have yet to find
an example where turning host verification off is necessary.

Closes #113.
@StefanKarpinski StefanKarpinski force-pushed the sk/no-verify-peer-only branch from f0e0c67 to 86e52d7 Compare April 20, 2021 21:04
@codecov-commenter
Copy link

codecov-commenter commented Apr 20, 2021

Codecov Report

Merging #114 (86e52d7) into master (db1d8d5) will decrease coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #114      +/-   ##
==========================================
- Coverage   93.00%   92.97%   -0.03%     
==========================================
  Files           5        5              
  Lines         486      484       -2     
==========================================
- Hits          452      450       -2     
  Misses         34       34              
Impacted Files Coverage Δ
src/Curl/Easy.jl 94.09% <ø> (-0.06%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update db1d8d5...86e52d7. Read the comment docs.

@StefanKarpinski
Copy link
Member Author

It seems that I can only trigger the lack-of-SNI failure (which this PR fixes) on macOS + Julia ≥ 1.6, which seems a little strange — I would expect this to be a problem everywhere or nowhere. @vdayanand, did you only encounter this issue on macOS or were you also able to trigger it on Linux/Windows?

@vdayanand
Copy link

vdayanand commented Apr 21, 2021

I can't reproduce this on linux. Looks like the difference here is the TLS library that is being used. Maybe we should try using LibreSSL like system curl?

Downloads.Curl version (Linux): libcurl/7.73.0 mbedTLS/2.24.0 zlib/1.2.11 libssh2/1.9.0 nghttp2/1.41.0
Downloads.Curl version (MacOS): libcurl/7.73.0 SecureTransport zlib/1.2.11 libssh2/1.9.0 nghttp2/1.41.0
System.Curl (MacOS): libcurl/7.64.1 (SecureTransport) LibreSSL/2.8.3 zlib/1.2.11 nghttp2/1.39.2

@StefanKarpinski
Copy link
Member Author

It seems ok to apply this patch and stop setting VERIFYHOST to zero. I can't find any actual example cases where this causes an insecure connection to be rejected. I think the server needs to be presenting a certificate for the wrong host entirely for this option to be required, but even with this patch, you can still connect to https://wrong.host.badssl.com which is, as I understand it supposed to be presenting a certificate for the wrong host. But maybe it's not wrong enough: it seems to be presenting a certificate for *.badssl.com which is maybe supposed to be rejected because the wildcard shouldn't apply to two levels of subdomain? I can't find an example of an HTTPS server that presents a certificate for the entirely wrong domain to see if that works.

@StefanKarpinski StefanKarpinski merged commit 6bddc0b into master Apr 21, 2021
@StefanKarpinski StefanKarpinski deleted the sk/no-verify-peer-only branch April 21, 2021 14:09
@mkitti
Copy link

mkitti commented Aug 18, 2021

In testing a conda-forge build of Julia 1.6.2 / Downloads.jl 1.4.1, it seems that libcurl 7.78.0 built with openssl 1.1.1k rather than mbedTLS throws an error when CURLOPT_SSL_VERIFYHOST is not set to 0. The error raised is

SSL: no alternative certificate subject name matches target host name 'wrong.host.badssl.com'

Further details can be found here:
conda-forge/julia-feedstock#119 (comment)

@StefanKarpinski
Copy link
Member Author

Well, that makes sense as I would expect that test to fail with this option unset and was a little puzzled why it didn't fail. It seems like this is a build error when using MbedTLS and the OpenSSL behavior is correct.

StefanKarpinski added a commit that referenced this pull request Aug 19, 2021
Since #114, we only turn
off peer verification, not host verification when the `SSL_NO_VERIFY`
variables are set. This means that the last set of tests in the "SSL no
verify override" testset *should* fail for `wrong.host.badssl.com`. That
is not what I was seeing, however — the test was still passing — which I
found puzzling but just moved on with my life at the time. It turns out
that the test *does* fail if libcurl is build with OpenSSL. Since
whether the test passes or not for that host depends on how things are
built, this change simply skips the test (by popping the URL from the
set of tested URLS for that testset).
StefanKarpinski added a commit that referenced this pull request Aug 19, 2021
Since #114, we only turn
off peer verification, not host verification when the `SSL_NO_VERIFY`
variables are set. This means that the last set of tests in the "SSL no
verify override" testset *should* fail for `wrong.host.badssl.com`. That
is not what I was seeing, however — the test was still passing — which I
found puzzling but just moved on with my life at the time. It turns out
that the test *does* fail if libcurl is build with OpenSSL. Since
whether the test passes or not for that host depends on how things are
built, this change simply skips the test (by popping the URL from the
set of tested URLS for that testset).

The tests above that which use the easy hook mechanism are fixed in a
different way: for those I made the hook disable both host and peer
verification, which should fix the tests for any bad host including when
the server sends the wrong host name.
StefanKarpinski added a commit that referenced this pull request Aug 19, 2021
Since #114, we only turn
off peer verification, not host verification when the `SSL_NO_VERIFY`
variables are set. This means that the last set of tests in the "SSL no
verify override" testset *should* fail for `wrong.host.badssl.com`. That
is not what I was seeing, however — the test was still passing — which I
found puzzling but just moved on with my life at the time. It turns out
that the test *does* fail if libcurl is build with OpenSSL. Since
whether the test passes or not for that host depends on how things are
built, this change simply skips the test (by popping the URL from the
set of tested URLS for that testset).

The tests above that which use the easy hook mechanism are fixed in a
different way: for those I made the hook disable both host and peer
verification, which should fix the tests for any bad host including when
the server sends the wrong host name.
ericphanson pushed a commit to ericphanson/Downloads.jl that referenced this pull request Jan 26, 2022
…liaLang#140)

Since JuliaLang#114, we only turn
off peer verification, not host verification when the `SSL_NO_VERIFY`
variables are set. This means that the last set of tests in the "SSL no
verify override" testset *should* fail for `wrong.host.badssl.com`. That
is not what I was seeing, however — the test was still passing — which I
found puzzling but just moved on with my life at the time. It turns out
that the test *does* fail if libcurl is build with OpenSSL. Since
whether the test passes or not for that host depends on how things are
built, this change simply skips the test (by popping the URL from the
set of tested URLS for that testset).

The tests above that which use the easy hook mechanism are fixed in a
different way: for those I made the hook disable both host and peer
verification, which should fix the tests for any bad host including when
the server sends the wrong host name.
ericphanson pushed a commit to ericphanson/Downloads.jl that referenced this pull request Jan 27, 2022
…liaLang#140)

Since JuliaLang#114, we only turn
off peer verification, not host verification when the `SSL_NO_VERIFY`
variables are set. This means that the last set of tests in the "SSL no
verify override" testset *should* fail for `wrong.host.badssl.com`. That
is not what I was seeing, however — the test was still passing — which I
found puzzling but just moved on with my life at the time. It turns out
that the test *does* fail if libcurl is build with OpenSSL. Since
whether the test passes or not for that host depends on how things are
built, this change simply skips the test (by popping the URL from the
set of tested URLS for that testset).

The tests above that which use the easy hook mechanism are fixed in a
different way: for those I made the hook disable both host and peer
verification, which should fix the tests for any bad host including when
the server sends the wrong host name.

(cherry picked from commit e22219f)
DilumAluthge added a commit that referenced this pull request Mar 3, 2022
* Before building and testing the package, make sure that the UUID has not been edited (#128)

(cherry picked from commit 21843d0)

* CI: Standardize the workflow for testing and changing the UUID (#129)

(cherry picked from commit cd002c3)

* fix #131 and add test (#132)

(cherry picked from commit adbb974)

* Improve inferability of download() (#133)

(cherry picked from commit 848d374)

* fix ci badge (#137)

(cherry picked from commit 3870614)

* Fix a handful of invalidations in expression-checking (#138)

ChainRulesCore defines `==(a, b::AbstractThunk)` and its converse,
and this invalidates a couple of poorly-typed Symbol checks.
This more "SSA-like" way of writing the code is easier to infer.

(cherry picked from commit 25f7af3)

* tests: skip wrong host test for SSL_NO_VERIFY (fix #139) (#140)

Since #114, we only turn
off peer verification, not host verification when the `SSL_NO_VERIFY`
variables are set. This means that the last set of tests in the "SSL no
verify override" testset *should* fail for `wrong.host.badssl.com`. That
is not what I was seeing, however — the test was still passing — which I
found puzzling but just moved on with my life at the time. It turns out
that the test *does* fail if libcurl is build with OpenSSL. Since
whether the test passes or not for that host depends on how things are
built, this change simply skips the test (by popping the URL from the
set of tested URLS for that testset).

The tests above that which use the easy hook mechanism are fixed in a
different way: for those I made the hook disable both host and peer
verification, which should fix the tests for any bad host including when
the server sends the wrong host name.

(cherry picked from commit e22219f)

* Fix input body size detection for IOBuffer(codeunits(str)) (#143)

Somewhat surprisingly, the type of this is not IOBuffer, but a related
type (Base.GenericIOBuffer{Base.CodeUnits{UInt8, String}}).

(cherry picked from commit 470b7f0)

* Typo fix: indiation -> indication (#144)

(cherry picked from commit 5f1509d)

* use Timer instead of libuv timer API

(cherry picked from commit 11493ff)

* use FDWatcher instead of libuv poll API

(cherry picked from commit 4c1d2af)

* fix wrong definition of curl_socket_t on Windows

(cherry picked from commit 2eb0491)

* Revert "stop using raw libuv API" (#156)

(cherry picked from commit c91876a)

* Revert "Revert "stop using raw libuv API" (#156)"

This reverts commit c91876a.

(cherry picked from commit 69acc13)

* add missing locks during Timer callbacks

(cherry picked from commit 43a3484)

* fix Timer usage (#158)

(cherry picked from commit 62b497e)

* Workaround for missing isopen check in FDWatcher (#161)

(possible multithread race with this still needs to be fixed)

(cherry picked from commit 7f91b8a)

* Check for timer isopen correctly (#162)

(cherry picked from commit 4250b35)

* remove trailing whitespace

(cherry picked from commit d8c626b)

* Avoid infinite recursion in `timer_callback` (#164)

Fixes #163

(cherry picked from commit a55825b)

* should also look into headers for input_size (#167)

If no content length is set while uploading some contents, Curl defaults to use
chunked transfer encoding. In some cases we want to prevent that because the
server may not support chunked transfers.

With this change, the request method will also look at the headers while
determining the input size and if found call `set_upload_size` as usual. So to
switch off chunked transfers, one must also know and set the content length
header while invoking `download` or `request` methods.

(cherry picked from commit ab628ab)

* rename: singularize add_{upload,seek}_callback

These only add one callback so having them be plural is weird.

(cherry picked from commit 5bd0826)

* add support for setting a debug callback

(cherry picked from commit 55a0c39)

* end-to-end tests for #167

This adds end-to-end tests for the changes introduced in #167.

Verbose mode is switched off for these tests, but switching it on would show that not setting content-length headers results in chunked transfer encoding while setting it prevents that. Both tests should pass.

(cherry picked from commit 911368d)

* tests: use debug option to test for non/chunked uploads

This combines the functionality from the previous two commits to not
only trigger both chunked and non-chunked uploads, but also test for
that difference by capturing and inspecting the debug events.

(cherry picked from commit 4e0408a)

* bump patch

Co-authored-by: Dilum Aluthge <dilum@aluthge.com>
Co-authored-by: Jakob Nybo Nissen <jakobnybonissen@gmail.com>
Co-authored-by: Yuto Horikawa <hyrodium@gmail.com>
Co-authored-by: Tim Holy <tim.holy@gmail.com>
Co-authored-by: Stefan Karpinski <stefan@karpinski.org>
Co-authored-by: Chris Foster <chris42f@gmail.com>
Co-authored-by: Benoît Legat <benoit.legat@gmail.com>
Co-authored-by: Jameson Nash <vtjnash@gmail.com>
Co-authored-by: Tanmay Mohapatra <tanmaykm@gmail.com>
StefanKarpinski added a commit that referenced this pull request Mar 24, 2022
Since #114, we only turn
off peer verification, not host verification when the `SSL_NO_VERIFY`
variables are set. This means that the last set of tests in the "SSL no
verify override" testset *should* fail for `wrong.host.badssl.com`. That
is not what I was seeing, however — the test was still passing — which I
found puzzling but just moved on with my life at the time. It turns out
that the test *does* fail if libcurl is build with OpenSSL. Since
whether the test passes or not for that host depends on how things are
built, this change simply skips the test (by popping the URL from the
set of tested URLS for that testset).

The tests above that which use the easy hook mechanism are fixed in a
different way: for those I made the hook disable both host and peer
verification, which should fix the tests for any bad host including when
the server sends the wrong host name.

(cherry picked from commit e22219f)
StefanKarpinski added a commit that referenced this pull request Mar 24, 2022
Since #114, we only turn
off peer verification, not host verification when the `SSL_NO_VERIFY`
variables are set. This means that the last set of tests in the "SSL no
verify override" testset *should* fail for `wrong.host.badssl.com`. That
is not what I was seeing, however — the test was still passing — which I
found puzzling but just moved on with my life at the time. It turns out
that the test *does* fail if libcurl is build with OpenSSL. Since
whether the test passes or not for that host depends on how things are
built, this change simply skips the test (by popping the URL from the
set of tested URLS for that testset).

The tests above that which use the easy hook mechanism are fixed in a
different way: for those I made the hook disable both host and peer
verification, which should fix the tests for any bad host including when
the server sends the wrong host name.

(cherry picked from commit e22219f)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants